feat(server): expose NetworkAutoDetect RTT via a shared handle#1346
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes the server’s NetworkAutoDetect RTT measurement via a shared Arc<AtomicU32> handle so display backends can read a fresh RTT value even after run() takes ownership of the server.
Changes:
- Add
autodetect_rtt: Arc<AtomicU32>toRdpServer, plus anautodetect_rtt_handle()getter and builder injectionwith_autodetect_rtt_handle(...). - Update the
AutoDetectRsphandler to store the latest measuredrtt_msinto the shared atomic handle. - Add tests to cover the
u32::MAXsentinel default and builder handle injection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crates/ironrdp-server/src/server.rs |
Adds the shared RTT handle to RdpServer, exposes it via an accessor, and updates it on auto-detect responses. |
crates/ironrdp-server/src/builder.rs |
Extends the builder to allow injecting a shared RTT handle used by both server and backend. |
crates/ironrdp-testsuite-core/tests/server/autodetect.rs |
Adds tests for the new RTT handle API and sentinel behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
This is looking good to me, but I have a question for you.
Using an Arc<AtomicU32> to share a state is of course perfectly fine, but don’t you end up polling the value repeatedly? I was wondering if a callback-based API would not be superior.
I don’t have the full consumer context, so maybe a simple Arc<Atomic*> is enough and does not perform poorly at all (e.g.: it’s optimal for a "read on a per-need basis model).
Greg Lamberson (glamberson)
force-pushed
the
feat/server-autodetect-rtt-handle
branch
from
177e183 to
4f5bd55
Compare
|
Apologies: I had no idea there was any activity here. I don't know what broke, but something did which made me miss some reviews. I'm reviewing this now... |
Greg Lamberson (glamberson)
force-pushed
the
feat/server-autodetect-rtt-handle
branch
from
4f5bd55 to
bef5fab
Compare
|
On the callback-based API idea: The value's read on demand, not polled: The backend samples it when it makes a flow-control decision (e.g., sizing the encoder in-flight window), so it's a single relaxed load at frame cadence, not a busy poll. That's the read-on-per-need case you describe where the atomic fits. A callback would invert the direction (server calling into the backend on each The other review threads are addressed and resolved above. |
The server measures network RTT in its AutoDetectManager, but after run() takes ownership a backend can no longer call rtt_snapshot(): the measurement is stranded in the running server. Mirror the existing display_suppressed handle so the value can be shared out. Add an Arc<AtomicU32> holding the latest measured RTT in milliseconds (u32::MAX until the first measurement), a with_autodetect_rtt_handle builder injector, and an autodetect_rtt_handle() getter. The AutoDetectRsp handler stores the measured rtt_ms into it. A display backend can then read a fresh, frame-traffic-independent network RTT for flow control without polling the server object. Drop the cfg_attr(egfx) gate on the new() too_many_arguments expect: the added parameter makes the non-egfx parameter count 8 (was 7), so the lint now fires in both feature configs and the expect is satisfied unconditionally.
Greg Lamberson (glamberson)
force-pushed
the
feat/server-autodetect-rtt-handle
branch
from
bef5fab to
410c138
Compare
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you, this sounds good to me then!
Benoît Cortier (CBenoit)
merged commit 481ea5d
into
Devolutions:master
Summary
The server measures network RTT in its AutoDetectManager (added in #1177), but once run() takes ownership a backend can no longer call rtt_snapshot() to read it: the measurement is stranded in the running server. This mirrors the existing display_suppressed shared-handle pattern (#1319) so the value can be shared out.
Adds an Arc holding the latest measured RTT in milliseconds (u32::MAX until the first measurement, and while auto-detect is disabled), a with_autodetect_rtt_handle builder injector, and an autodetect_rtt_handle() getter. The AutoDetectRsp handler stores the measured rtt_ms into it. A display backend can then read a fresh, frame-traffic-independent network RTT for flow control (for example to size an encoder's in-flight window) without polling the server object.
This also drops the cfg_attr(egfx) gate on new()'s too_many_arguments expect: the added parameter makes the non-egfx parameter count 8 (it was exactly 7, where the lint was silent), so the lint now fires in both feature configs and the expect is satisfied unconditionally.
Validation
cargo xtask check fmt/lints/tests/typos/locks all pass. New tests in ironrdp-testsuite-core cover the u32::MAX default and that with_autodetect_rtt_handle round-trips the same Arc.
Notes
Additive; mirrors #1319's handle pattern. new() gains a parameter the same way #1319 added display_suppressed (feat, not feat!, per that precedent). Exposes only the generic RTT value; no flow-control policy.